Throwing an exception in case the sum of weights is over uint32_t max value by adisuissa · Pull Request #10578 · envoyproxy/envoy

adisuissa · 2020-03-30T21:04:28Z

Verifying that the sum of weights in the configuration file isn't over uint32_t maximal value.
If the sum is overflowing, an exception is thrown.

Risk Level: Low
Testing: None
Fixes fuzz test: 5702999713513472

Signed-off-by: Adi Suissa-Peleg adip@google.com

… value Signed-off-by: Adi Suissa-Peleg <adip@google.com>

adisuissa · 2020-03-30T21:07:16Z

/cc @asraa @snowp
If you think of a better way to verify overflow, or if there's a way to validate it using proto validators, please let me know.
Also, couldn't find the unit-tests of the weights configuration, so if there is a file that you are aware of, I'll add new tests to it.

snowp

Thanks this looks like a reasonable fix to me.

snowp · 2020-03-31T18:27:33Z

source/common/upstream/thread_aware_lb_impl.cc

+  uint64_t sum = 0;
  for (const auto& host : hosts) {
    sum += host->weight();
+    if (sum > std::numeric_limits<uint32_t>::max()) {


This seems like a fine way to check for overflow (the other one I could think of would be if (sum + host->weight() >= sum) with sum being uint32_t).

Can you make sure both this and the other exception has coverage and if not add a unit test?

Thanks @snowp !
I agree that we can check if sum is increasing, but I think the current option clarifies that we check for an overflow.

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

adisuissa · 2020-04-01T18:19:44Z

Added tests for the new exceptions

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

snowp

Thanks this looks good, just a comment on the error message

snowp · 2020-04-02T18:28:24Z

source/common/upstream/thread_aware_lb_impl.cc

    sum += weight;
+    if (sum > std::numeric_limits<uint32_t>::max()) {
+      throw EnvoyException(
+          fmt::format("Load balancing locality weights has accumulated over its maximum value {}",


I think this error message could be a bit clearer: it might not be obvious to an operator that these weights are being accumulated. Maybe the message should be around the fact that the sum of locality weights within a priority causing overflow? Similar with host weights, though I'm not sure what the invariant is there (per locality? per priority?)

snowp · 2020-04-02T20:06:30Z

Also: @envoyproxy/api-shepherds is this something we'd want to be documenting on the protos? Or are we okay with this just failing validation if it happens?

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

adisuissa · 2020-04-03T02:05:20Z

Changed the error messages according to:

envoy/api/envoy/api/v2/endpoint/endpoint_components.proto

Lines 93 to 101 in b5a3405

    
           // The optional load balancing weight of the upstream host; at least 1. 
        
           // Envoy uses the load balancing weight in some of the built in load 
        
           // balancers. The load balancing weight for an endpoint is divided by the sum 
        
           // of the weights of all endpoints in the endpoint's locality to produce a 
        
           // percentage of traffic for the endpoint. This percentage is then further 
        
           // weighted by the endpoint's locality's load balancing weight from 
        
           // LocalityLbEndpoints. If unspecified, each host is presumed to have equal 
        
           // weight in a locality. 
        
           google.protobuf.UInt32Value load_balancing_weight = 4 [(validate.rules).uint32 = {gte: 1}];

and

envoy/api/envoy/api/v2/endpoint/endpoint_components.proto

Lines 116 to 126 in b5a3405

    
           // Optional: Per priority/region/zone/sub_zone weight; at least 1. The load 
        
           // balancing weight for a locality is divided by the sum of the weights of all 
        
           // localities  at the same priority level to produce the effective percentage 
        
           // of traffic for the locality. 
        
           // 
        
           // Locality weights are only considered when :ref:`locality weighted load 
        
           // balancing <arch_overview_load_balancing_locality_weighted_lb>` is 
        
           // configured. These weights are ignored otherwise. If no weights are 
        
           // specified when locality weighted load balancing is enabled, the locality is 
        
           // assigned no load. 
        
           google.protobuf.UInt32Value load_balancing_weight = 3 [(validate.rules).uint32 = {gte: 1}];

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

htuch · 2020-04-03T20:54:14Z

@snowp it probably makes sense to document. We can't use PGV though, you'd need something like CEL to express cross-field constraints which we don't have today.

@adisuissa can you ad a comment to https://github.com/envoyproxy/envoy/blob/master/api/envoy/api/v2/endpoint/endpoint_components.proto#L93 and https://github.com/envoyproxy/envoy/blob/master/api/envoy/api/v2/endpoint/endpoint_components.proto#L121 to make this explicit in the API docs? Thanks.

adisuissa · 2020-04-03T20:58:15Z

@snowp it probably makes sense to document. We can't use PGV though, you'd need something like CEL to express cross-field constraints which we don't have today.

@adisuissa can you ad a comment to https://github.com/envoyproxy/envoy/blob/master/api/envoy/api/v2/endpoint/endpoint_components.proto#L93 and https://github.com/envoyproxy/envoy/blob/master/api/envoy/api/v2/endpoint/endpoint_components.proto#L121 to make this explicit in the API docs? Thanks.

Sure.
I'm assuming it also needs to be added to v3 (probably will be handled by proto_format), but if I'm wrong please correct me.

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

…pi_shadow) Signed-off-by: Adi Suissa-Peleg <adip@google.com>

repokitteh-read-only · 2020-04-04T02:48:17Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #10578 was synchronize by adisuissa.

see: more, trace.

htuch · 2020-04-05T23:54:30Z

/lgtm api

adisuissa · 2020-04-06T14:19:27Z

/retest

repokitteh-read-only · 2020-04-06T14:19:31Z

🔨 rebuilding ci/circleci: coverage (failed build)

🐱

Caused by: a #10578 (comment) was created by @adisuissa.

see: more, trace.

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

…20200330 Signed-off-by: Adi Suissa-Peleg <adip@google.com>

snowp

LGTM. @htuch this needs another API sign off

mattklein123

Please merge master so the v4alpha changes propagate.

…20200330 Signed-off-by: Adi Suissa-Peleg <adip@google.com>

adisuissa · 2020-04-08T19:55:55Z

Please merge master so the v4alpha changes propagate.

merged latest master.

Throwing an exception in case the sum of weights is over uint32_t max…

94737cc

… value Signed-off-by: Adi Suissa-Peleg <adip@google.com>

htuch assigned snowp Mar 31, 2020

snowp suggested changes Mar 31, 2020

View reviewed changes

Fixing exception text, and adding tests

ef56589

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

Fixing format

358920b

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

snowp suggested changes Apr 2, 2020

View reviewed changes

Fixing error messages

2fa1d19

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

Adding the fixed tests

0abf123

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

adisuissa added 2 commits April 3, 2020 22:39

Updating the proto description to describe the sum limit

64f85a8

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

Updating the proto description to describe the sum limit (generated_a…

bf98761

…pi_shadow) Signed-off-by: Adi Suissa-Peleg <adip@google.com>

repokitteh-read-only bot added the api label Apr 4, 2020

repokitteh-read-only bot removed the api label Apr 5, 2020

adisuissa added 2 commits April 7, 2020 22:14

merging upstream/master

e9bba69

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

Merge remote-tracking branch 'upstream/master' into fix_oss_fuzz_bug_…

6cca6ec

…20200330 Signed-off-by: Adi Suissa-Peleg <adip@google.com>

repokitteh-read-only bot added the api label Apr 8, 2020

snowp approved these changes Apr 8, 2020

View reviewed changes

mattklein123 requested changes Apr 8, 2020

View reviewed changes

mattklein123 added the waiting label Apr 8, 2020

Merge remote-tracking branch 'upstream/master' into fix_oss_fuzz_bug_…

79beb32

…20200330 Signed-off-by: Adi Suissa-Peleg <adip@google.com>

repokitteh-read-only bot removed the waiting label Apr 8, 2020

mattklein123 approved these changes Apr 8, 2020

View reviewed changes

repokitteh-read-only bot removed the api label Apr 8, 2020

mattklein123 merged commit e1df07e into envoyproxy:master Apr 8, 2020

Conversation

adisuissa commented Mar 30, 2020

Uh oh!

adisuissa commented Mar 30, 2020

Uh oh!

snowp left a comment

Choose a reason for hiding this comment

Uh oh!

snowp Mar 31, 2020

Choose a reason for hiding this comment

Uh oh!

adisuissa Apr 1, 2020

Choose a reason for hiding this comment

Uh oh!

adisuissa commented Apr 1, 2020

Uh oh!

snowp left a comment

Choose a reason for hiding this comment

Uh oh!

snowp Apr 2, 2020

Choose a reason for hiding this comment

Uh oh!

snowp commented Apr 2, 2020

Uh oh!

adisuissa commented Apr 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

htuch commented Apr 3, 2020

Uh oh!

adisuissa commented Apr 3, 2020

Uh oh!

repokitteh-read-only bot commented Apr 4, 2020

Uh oh!

htuch commented Apr 5, 2020

Uh oh!

adisuissa commented Apr 6, 2020

Uh oh!

repokitteh-read-only bot commented Apr 6, 2020

Uh oh!

snowp left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

adisuissa commented Apr 8, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

adisuissa commented Apr 3, 2020 •

edited

Loading